-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Reconciler Extensions #157
base: main
Are you sure you want to change the base?
New Reconciler Extensions #157
Conversation
…tors with customized reconciliation workflows. Shows how pre/post hooks can be implemented in terms of extensions. Signed-off-by: Moritz Clasmeier <moritz@stackrox.com>
// WithExtension is an Option that registers an extension into the reconciler. | ||
// For example, extensions may be necessary when need arises to manage certain | ||
// resources outsides of the standard Helm-based workflow. | ||
func WithExtension(e extension.ReconcilerExtension) Option { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would mean that if we have to add any additional extension points in future, then there would be a user facing breaking change in extension.ReconcilerExtension
since we would be adding additional sub-interfaces. It would be helpful if we can implement this to avoid breaking changes in future when we have to introduce more extensions. Would something like this work?
// 1. We have interfaces defined for extension points as mentioned in this PR:
// BeginReconciliationExtension defines the extension point to execute at the beginning of a reconciliation flow.
type BeginReconciliationExtension interface {
BeginReconcile(ctx context.Context, reconciliationContext *Context, obj *unstructured.Unstructured) error
}
// EndReconciliationExtension defiens the extension point to execute at the end of a reconciliation flow.
type EndReconciliationExtension interface {
EndReconcile(ctx context.Context, reconciliationContext *Context, obj *unstructured.Unstructured) error
}
// 2. We have an internal struct that encapsulates this
type allextensions interface {
BeginExts []BeginReconciliationExtension
EndExts []EndReconciliationExtension
}
// 3. Reconciler in turn encapsulates the above `allextensions`
type Reconciler struct {
....
extensions allextensions
....
}
// 4. WithExtensions accepts interfaces and we type assert here to check which extension point it belongs to.
func WithExtensions(ext ...interface{}) Option {
return func(r *Reconciler) error {
for _, e := range ext {
if b, ok := e.(BeginReconciliationExtension); ok {
r.extensions.BeginExts = append(r.extensions.BeginExts, b)
}
if end, ok := e.(EndReconciliationExtension); ok {
r.extensions.EndExts = append(r.extensions.EndExts, end)
}
}
return nil
}
}
As we keep on adding extension points, we can expand WithExtensions
to type assert on each of them without causing user facing breaking changes.
This does look convoluted and I am not sure if it is the best way. Please feel free to disregard this approach, but it would be nice if we can come up with something which would cause minimal breaking changes later.
// Context can be used for providing extension methods will more context about the reconciliation flow. | ||
// This contains data objects which can be useful for specific extensions but might not be required for all. | ||
type Context struct { | ||
KubernetesConfig *rest.Config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the users need to know the rest.Config
entirely? Wouldn't it be sufficient if we pass just the client instead?
|
||
// NoOpReconcilerExtension implements all extension methods as no-ops and can be used for convenience | ||
// when only a subset of the available methods needs to be implemented. | ||
type NoOpReconcilerExtension struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need NoOpReconcilerExtension
? Instead can we can let the users handle this, since as we keep on adding extension points, we would have to keep updating this. In an ideal scenario, we would not want breaking changes when we add extension points, but in case we still end up having predefined interfaces, would this be necessary?
@mtesseract: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This PR demonstrates on implementation strategy for extensions.
Looking forward to reviews & discussion.
Based on the discussions we've had, I'd like to comment on a few aspects:
It was suggested to go forward with a minimal interface that would suit our needs: The new interface is minimal in the sense that it only contains two methods, similar to the PreHook/PostHook mechanism.
This proposal tries to keep most of the advantages of static typing but in order to make future extensibility easy there is a
NoOp
value, which can be used for inheriting no-op-default-implementations for extension methods.The nomenclature has been changed: It's not anymore pre- & post-reconcile, but rather begin- & end-reconcile, to highlight that these custom steps are still to be regarded as part of the overall reconciliation workflow.
The logger is now injected into the context as done in the kubebuilder scaffolding.
In this proposal I am deliberately not using
context.Context
for untyped injection of more data potentially required by extensions because the understanding I have acquired from my research suggests that this is in rather sharp contradiction to the current best-practices. For instance, the official documentation says "Use context Values only for request-scoped data that transits processes and APIs, not for passing optional parameters to functions.". This is not 100% our use-case, but I understand it so that also our use-case would be questionable. This is against what we have discussed in our meeting. What I have done instead is I created a dedicated "reconciliation context", that is passed to all extension methods and is distinct fromcontext.Context
but used for providing all sorts of potentially-available context for the reconciliation workflow. My hope here is that this would allow for easy extensibility in the future and would help us to not box ourselves in.I have also shown how to implement PreHooks and PostHooks using the general extension interface. This compatibility code could remain, but doesn't have to remain.
I will be back from vacation on April 4, opening this draft so that you can already have a look at the current state. @varshaprasad96 @joelanford @misberner